[Local catalog] Use specific skip reasons in analytics #16382
Conversation
joshheald
added
type: task
|
|
iamgabrielma
left a comment
iamgabrielma
left a comment
There was a problem hiding this comment.
LGTM!
First sync:
🔵 Tracked pos_local_catalog_sync_skipped, properties: [sync_type: full, reason: pos_not_opened_30_days]
Subsequent sync:
🔵 Tracked pos_local_catalog_sync_skipped, properties: [reason: pos_not_opened_30_days, sync_type: incremental]
| case .notOpenedRecently: "pos_not_opened_30_days" | ||
| case .neverOpenedAndPastFirstSyncGracePeriod: "pos_not_opened_30_days" |
There was a problem hiding this comment.
Is it ok to use the same pos_not_opened_30_days value for both cases notOpenedRecently and neverOpenedAndPastFirstSyncGracePeriod? I noticed it is expected by the coordinator tests, but raising it just in case 👍
There was a problem hiding this comment.
Yep, they're basically the same thing
| switch try await syncEligibility(for: siteID) { | ||
| case .eligible(reason: let reason): | ||
| switch reason { | ||
| case .neverSynced: | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (no first sync date recorded)") | ||
| case .openedRecently(daysSinceOpened: let daysSinceOpened): | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (last opened \(daysSinceOpened) days ago)") | ||
| case .recentFirstSync(daysSinceFirstSync: let daysSinceFirstSync): | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) eligible (within grace period: \(daysSinceFirstSync) days since first sync)") | ||
| } | ||
| return true | ||
| case .ineligible(reason: let reason): | ||
| switch reason { | ||
| case .notEligibleForLocalCatalog: | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) - Catalog ineligible") | ||
| case .notOpenedRecently(daysSinceOpened: let daysSinceOpened): | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (POS last opened \(daysSinceOpened) days ago)") | ||
| case .neverOpenedAndPastFirstSyncGracePeriod(daysSinceFirstSync: let daysSinceFirstSync): | ||
| DDLogInfo("📋 POSCatalogSyncCoordinator: Site \(siteID) ineligible (past 30-day grace period, " + | ||
| "no recent POS open, \(daysSinceFirstSync) days since first sync)") | ||
| } |
There was a problem hiding this comment.
Do we need these DDLogInfo? If we don't, we could either return the bool directly or remove the function entirely (depending on the other usages of syncEligibility(for siteID: Int64) async throws -> SyncEligibility)
func checkSyncEligibility(for siteID: Int64) async throws -> Bool {
switch try await syncEligibility(for: siteID) {
case .eligible:
return true
case .ineligible:
return false
}
}
There was a problem hiding this comment.
They're the pre-existing, I just moved them to reuse the underlying logic, and they're pretty helpful to figure out what's going on...
joshheald
deleted the
woomob-1763-local-catalog-use-specific-skip-reasons-in-analytics
branch
Description
This PR updates the
skipReasonwe track to includepos_not_opened_30_days. The skip event also gets async_typeproperty.Test Steps
Ensure you're using local catalog by setting the app version to 23.8 in the project file.
Replace
.dayswith.secondsinPOSCatalogSyncCoordinator.syncEligibility(for:)to speed up time.Assuming you've previously synced, you'll see the event below tracked when you launch the app.
pos_not_eligibleis a theoretical event, and doesn't get tracked, because we don't set up all the infrastructure in that case.Screenshots
RELEASE-NOTES.txtif necessary.